Skip to content

Conversation

Shashankss1205
Copy link
Contributor

@Shashankss1205 Shashankss1205 commented Mar 1, 2024

Resolves #1092 .

Description

What is the purpose of this pull request?

There's an issue with prevGraphemeClusterBreak package, if I compare the results with nextGraphemeClusterBreak for a single skin tone emoji it doesn't return the same values,

prevGraphemeClusterBreak( '👉🏿' ) // returns 1
nextGraphemeClusterBreak( '👉🏿' ) // returns -1

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

Copy link
Contributor

@stdlib-bot stdlib-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Hi there! 👋

And thank you for opening your first pull request! We will review it shortly. 🏃 💨

@Shashankss1205
Copy link
Contributor Author

@kgryte , @Pranavchiku, @Planeshifter Please check the integrity of this PR, and send reviews

@kgryte kgryte added the Needs Review A pull request which needs code review. label Mar 9, 2024
// Get the corresponding grapheme break and emoji properties:
breaks.push( breakProperty( cp ) );
emoji.push( emojiProperty( cp ) );
if (!(cp >= 0x1F3FB && cp <= 0x1F3FF)) { // Check if the character is not a skin tone modifier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too narrow a patch. It is not just skin tones, TMK, but any modifier. The implementation more generally should be refactored to ensure that grapheme clusters comprised of multiple code points are handled correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I am sorry, I got confused by seeing the original issue being raised about the colour tone.
I will try to debug and propose a generic solution to this as soon as possible.
Thank You.

// Get the corresponding grapheme break and emoji properties:
breaks.push( breakProperty( cp ) );
emoji.push( emojiProperty( cp ) );
if (!(cp >= 0x1F3FB && cp <= 0x1F3FF)) { // Check if the character is not a skin tone modifier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Mar 9, 2024
@kgryte kgryte added Needs Changes Pull request which needs changes before being merged. and removed Needs Changes Pull request which needs changes before being merged. Needs Review A pull request which needs code review. labels Mar 17, 2024
@Planeshifter Planeshifter added the Potential Duplicate There might be another pull request resolving the same issue. label Mar 15, 2025
@kgryte kgryte added the autoclose: Stale Pull request which should be auto-closed as considered stale. label Apr 3, 2025
@stdlib-bot
Copy link
Contributor

This pull request has been automatically closed because it has been inactive for an extended period after changes were requested. If you still wish to pursue this contribution, feel free to reopen the pull request or submit a new one.

We appreciate your interest in contributing to stdlib!

@stdlib-bot stdlib-bot closed this Apr 3, 2025
@stdlib-bot stdlib-bot added the Good First PR A pull request resolving a Good First Issue. label Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autoclose: Stale Pull request which should be auto-closed as considered stale. Good First PR A pull request resolving a Good First Issue. Needs Changes Pull request which needs changes before being merged. Potential Duplicate There might be another pull request resolving the same issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug with skin tone emojis when using prevGraphemeClusterBreak package

4 participants